Closed
Bug 1047267
Opened 10 years ago
Closed 10 years ago
Move EXTRA_LIBS to moz.build
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla34
People
(Reporter: glandium, Assigned: glandium)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(5 files, 1 obsolete file)
2.81 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
13.53 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
6.87 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
83.25 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
1.86 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8468346 -
Flags: review?(gps)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8468354 -
Flags: review?(gps)
Assignee | ||
Comment 3•10 years ago
|
||
The parts in config/external/* are kind of awkward, especially those related to MOZ_FOLD_LIBS, but I don't have any better idea until we have more expressiveness in moz.build and/or we move nss/nspr to moz.build. Note there are interesting subtle differences between what was in Makefile.in and what ands up in backend.mk through what's added as a replacement to moz.build, but in practice, there's no significant difference. The most funny one is in media/mtransport/test, where you'd think libssl is being statically linked even in the MOZ_FOLD_LIBS case before this patch, but it actually wasn't (the joys of static libraries magically being dropped).
Attachment #8468542 -
Flags: review?(gps)
Assignee | ||
Comment 4•10 years ago
|
||
Sadly, even after this queue we can't remove EXTRA_LIBS and OS_LIBS just yet. EXTRA_LIBS because of some hackish thing in tools/trace-malloc, and the stlport stuff in some test Makefiles (which, in fact, I think can just be removed, but I'll do that in a followup), and OS_LIBS because it's defined in autoconf.mk (we'd need to move the _MOZBUILD_EXTERNAL_VARIABLES stuff to baseconfig.mk and change some of the tests, I'll leave that to a followup too)
Attachment #8468545 -
Flags: review?(gps)
Assignee | ||
Comment 5•10 years ago
|
||
Heh, turns out there's actually one test in mtransport that does use a small part of libssl.a because the corresponding symbols are not exported from the folded libnss, and that only links if libssl.a comes after the folded libnss, which is the kind of setup moz.build doesn't support. I just exported the missing symbols, instead of pretending adding libssl.a had actually more effect than it has and add workarounds for that. There were also missing ifs in the top-level moz.build.
Attachment #8468586 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8468542 -
Attachment is obsolete: true
Attachment #8468542 -
Flags: review?(gps)
Updated•10 years ago
|
Attachment #8468346 -
Flags: review?(gps) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8468354 [details] [diff] [review] Allow to reference libraries from third-party build systems in USE_LIBS Review of attachment 8468354 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +284,4 @@ > raise SandboxValidationError( > '%s contains "%s", but there is no "%s" %s in %s.' > % (variable, path, name, > self.LIBRARY_NAME_VAR[obj.KIND], dir), sandbox) Adding a test that verifies a USE_LIBS pointing to a non-external directory would be nice. But this code looks solid. @@ +363,5 @@ > + > + if force_static: > + return ExternalStaticLibrary(sandbox, name) > + else: > + return ExternalSharedLibrary(sandbox, name) Clever. @@ +992,5 @@ > o.dirs = sandbox.get('DIRS', []) > o.test_dirs = sandbox.get('TEST_DIRS', []) > o.affected_tiers = sandbox.get_affected_tiers() > > + self._external_paths -= set([o.relobjdir]) It isn't obvious to me why this is needed. I'm not too concerned about its presence, but this really needs an inline comment. You could use the set literal syntax if you wanted: -= {o.relobjdir}
Attachment #8468354 -
Flags: review?(gps) → review+
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #6) > It isn't obvious to me why this is needed. We have subconfigured things that are still handled by moz.build (like jemalloc3)
Assignee | ||
Comment 8•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=439ac565d271
Comment 9•10 years ago
|
||
Comment on attachment 8468586 [details] [diff] [review] Move remaining OS_LIBS and EXTRA_LIBS to moz.build Review of attachment 8468586 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/app/moz.build @@ -24,5 @@ > > - if not CONFIG['MOZ_NATIVE_ZLIB'] and not CONFIG['ZLIB_IN_MOZCONFIG']: > - USE_LIBS += [ > - 'mozz', > - ] I like how library names are unified in the new world! ::: config/external/sqlite/moz.build @@ +8,5 @@ > + > +if CONFIG['MOZ_NATIVE_SQLITE']: > + OS_LIBS += CONFIG['SQLITE_LIBS'] > +else: > + DIRS += ['../../../db/sqlite3/src'] Can't this be '/db/sqlite3/src'? ::: config/external/zlib/moz.build @@ +16,5 @@ > + # 'mozglue' > + # ] > + pass > + DIRS += [ > + '../../../modules/zlib', '/modules/zlib' ? ::: content/media/webaudio/compiledtest/moz.build @@ +17,5 @@ > ] > > USE_LIBS += [ > 'mozalloc', > + 'nspr', The case for templates is quickly growing. ::: toolkit/library/Makefile.in @@ -4,5 @@ > > include $(topsrcdir)/toolkit/library/libxul.mk > -# Please don't remove the following comment, because it does some magic: > -# libxul.mk adds FT2_LIBS, NSS_LIBS and NSPR_LIBS, but we want to have the > -# string in this file to trigger a dependency on freetype2, nss and nspr. Can we remove the code that looks for this in the backend yet?
Attachment #8468586 -
Flags: review?(gps) → review+
Comment 10•10 years ago
|
||
Comment on attachment 8468545 [details] [diff] [review] Remove the trigger hacks added in bug 1043344 Review of attachment 8468545 [details] [diff] [review]: ----------------------------------------------------------------- And this answers my question from the last review. Thanks for splitting the patches into self-contained work. It makes my life as reviewer much easier. I should have paid better attention to the patch descriptions. ::: python/mozbuild/mozbuild/frontend/reader.py @@ -298,5 @@ > raise Exception('Directory has already been registered with ' > 'tier: %s' % path) > > - path = PathWithTrigger(path) > - path.trigger = trigger I'm pretty sure there is a "trigger" argument to this method that still needs deleted.
Attachment #8468545 -
Flags: review?(gps) → review+
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #9) > Comment on attachment 8468586 [details] [diff] [review] > Move remaining OS_LIBS and EXTRA_LIBS to moz.build > > Review of attachment 8468586 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: b2g/app/moz.build > @@ -24,5 @@ > > > > - if not CONFIG['MOZ_NATIVE_ZLIB'] and not CONFIG['ZLIB_IN_MOZCONFIG']: > > - USE_LIBS += [ > > - 'mozz', > > - ] > > I like how library names are unified in the new world! > > ::: config/external/sqlite/moz.build > @@ +8,5 @@ > > + > > +if CONFIG['MOZ_NATIVE_SQLITE']: > > + OS_LIBS += CONFIG['SQLITE_LIBS'] > > +else: > > + DIRS += ['../../../db/sqlite3/src'] > > Can't this be '/db/sqlite3/src'? Sadly no. There are plenty of variables that don't support those paths, and DIRS is one: The error occurred while processing the following file: /home/glandium/mozilla-central/config/external/sqlite/moz.build The error occurred when validating the result of the execution. The reported error is: Attempting to process file outside of allowed paths: /db/sqlite3/src/moz.build That said, iirc the patch from bug 1035599 would make it work. > ::: content/media/webaudio/compiledtest/moz.build > @@ +17,5 @@ > > ] > > > > USE_LIBS += [ > > 'mozalloc', > > + 'nspr', > > The case for templates is quickly growing. And that's next on my list.
Assignee | ||
Comment 12•10 years ago
|
||
With bug 1050037 and bug 1050029, this allows to forbis OS_LIBS and EXTRA_LIBS.
Attachment #8468978 -
Flags: review?(gps)
Comment 13•10 years ago
|
||
Comment on attachment 8468978 [details] [diff] [review] To fold with "Move remaining OS_LIBS and EXTRA_LIBS to moz.build" Review of attachment 8468978 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/trace-malloc/Makefile.in @@ +23,5 @@ > > +# This is the last use of EXTRA_LIBS, and it would be sad to not have an > +# error if it's used in other Makefiles just because of it. So hack around > +# the check. > +_DEPRECATED_VARIABLES := $(filter-out EXTRA_LIBS,$(_DEPRECATED_VARIABLES)) It saddens me this hack works. I hope it doesn't get cargo culted.
Attachment #8468978 -
Flags: review?(gps) → review+
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d00374d813 https://hg.mozilla.org/integration/mozilla-inbound/rev/698ae2f2c8bf https://hg.mozilla.org/integration/mozilla-inbound/rev/6b285759568c https://hg.mozilla.org/integration/mozilla-inbound/rev/af7e67f1af51
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/12d00374d813 https://hg.mozilla.org/mozilla-central/rev/698ae2f2c8bf https://hg.mozilla.org/mozilla-central/rev/6b285759568c https://hg.mozilla.org/mozilla-central/rev/af7e67f1af51
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
QA Whiteboard: [qa-]
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•